Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow custom configuration #243

Merged
merged 6 commits into from
May 15, 2024
Merged

Allow custom configuration #243

merged 6 commits into from
May 15, 2024

Conversation

JesusSilvaUtrera
Copy link
Contributor

@JesusSilvaUtrera JesusSilvaUtrera commented May 9, 2024 •
edited
Loading

🎉 New feature

Closes #135 and #136

Summary

I have continued the work on these issues to add the possibility to pass arguments to the xacro file directly from the launch file. I created a hardware.yaml in the config folder to load the params from there instead of hardcoding them in the andino_control.xacro file.

I have also updated the README.md with these new features.

Test it

You can test that it works running ros2 launch andino_description view_andino.launch.py, which executes RViz and the robot_state_publisher to check that everything remains as before, but now more parameterized.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@JesusSilvaUtrera
Hardware configuration is now being loaded from YAML file, updated RE…
4b82302 
…ADME and updated launch files to pass xacro arguments

Signed-off-by: JesusSilvaUtrera <[email protected]>
@JesusSilvaUtrera
Copy link
Contributor Author

The arguments for the xacro are being passed directly inside the launch file, not as arguments of the launch files (I don't know if this was the expected result, IMO it is easier to understand this way, just changing the path in the launch file).

francocipollone
francocipollone reviewed May 13, 2024
View reviewed changes
Copy link
Collaborator

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution

andino_description/urdf/include/andino_control.urdf.xacro Show resolved Hide resolved
@francocipollone
Copy link
Collaborator

The arguments for the xacro are being passed directly inside the launch file, not as arguments of the launch files (I don't know if this was the expected result, IMO it is easier to understand this way, just changing the path in the launch file).

That's ok and also expected behavior.
From a user perspective it is useful to be able to indicate other .yaml file to be used instead the default one, however the parameters are always read from a yaml file.

jballoffet
jballoffet requested changes May 15, 2024
View reviewed changes
Copy link
Member

@jballoffet jballoffet left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall changes LGTM, I left some comments, PTAL. Thanks!

@JesusSilvaUtrera
Copy link
Contributor Author

Overall changes LGTM, I left some comments, PTAL. Thanks!

One little question: Franco left a comment about maybe moving the andino_control.urdf.xacro to andino_control package, but I think it is better to leave it here, as that would create a dependency between andino_description and andino_control. wdyt?

@JesusSilvaUtrera
Addressed Javi comments in the PR
fd451a2 
Signed-off-by: JesusSilvaUtrera <[email protected]>
@jballoffet
Copy link
Member

Overall changes LGTM, I left some comments, PTAL. Thanks!

One little question: Franco left a comment about maybe moving the andino_control.urdf.xacro to andino_control package, but I think it is better to leave it here, as that would create a dependency between andino_description and andino_control. wdyt?

Good question. Let's keep it as is on this PR and if needed, send another PR for moving the xacro file to the andino_control package.

jballoffet
jballoffet approved these changes May 15, 2024
View reviewed changes
Copy link
Member

@jballoffet jballoffet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @JesusSilvaUtrera!

@jballoffet jballoffet merged commit 9f495b8 into Ekumen-OS:humble May 15, 2024
4 checks passed
@JesusSilvaUtrera JesusSilvaUtrera deleted the jesus/#135_136_custom_configuration_in_description branch May 15, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jballoffet jballoffet jballoffet approved these changes

@francocipollone francocipollone Awaiting requested review from francocipollone francocipollone is a code owner

Assignees
No one assigned
Labels
None yet
Projects
Andino Roadmap
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow to load custom configuration for the description
3 participants
@JesusSilvaUtrera @francocipollone @jballoffet